Skip to content
This repository was archived by the owner on Oct 16, 2020. It is now read-only.

feat(typescript): upgrade to typescript 2.7.2 #431

Merged
merged 1 commit into from
Feb 16, 2018
Merged

feat(typescript): upgrade to typescript 2.7.2 #431

merged 1 commit into from
Feb 16, 2018

Conversation

daviwil
Copy link
Contributor

@daviwil daviwil commented Feb 16, 2018

/cc @damieng

daviwil
daviwil commented Feb 16, 2018
View reviewed changes
src/test/typescript-service-helpers.ts
@@ -382,7 +382,7 @@ export function describeTypeScriptService(
character: 8,
},
},
contents: [{ language: 'typescript', value: 'import Foo' }, '**alias**'],
contents: [{ language: 'typescript', value: 'class Foo\nimport Foo' }, '**alias**'],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like hovers now give you more symbol info than just the import statement!

@codecov
Copy link

codecov bot commented Feb 16, 2018 •
edited
Loading

Codecov Report

Merging #431 into master will decrease coverage by 0.09%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #431     +/-   ##
=========================================
- Coverage   83.26%   83.16%   -0.1%     
=========================================
  Files          15       15             
  Lines        2038     2038             
  Branches      482      415     -67     
=========================================
- Hits         1697     1695      -2     
- Misses        339      341      +2     
  Partials        2        2
Impacted Files Coverage Δ
src/project-manager.ts 87.26% <0%> (-0.54%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2cac6cb...86f0842. Read the comment docs.

@daviwil
Copy link
Contributor Author

daviwil commented Feb 16, 2018

Anything I can/should do about the drop in code coverage?

@felixfbecker
Copy link
Contributor

Hmm, seems like getNewLine() is not called anymore by TS. Could you check if it is still part of the interface? If not, we can safely remove it

https://codecov.io/gh/sourcegraph/javascript-typescript-langserver/pull/431/changes#L95

@daviwil
Copy link
Contributor Author

daviwil commented Feb 16, 2018

Still part of the interface: https://github.com/Microsoft/TypeScript/blob/v2.7.2/src/services/types.ts#L163

Looks like there was a recent change to prioritize the use of FormatCodeSettings if it's provided before ever calling getNewLine(), so that could account for the lack of calls:

https://github.com/Microsoft/TypeScript/pull/21158/files#diff-183e2291cbb94d65ae0406354ce1ba7aR1262

I'll remove that function from the language service host and see what happens!

@felixfbecker
Copy link
Contributor

In that case just leave it in and not worry about the coverage

@daviwil
Copy link
Contributor Author

daviwil commented Feb 16, 2018

Ok, sounds good!

@felixfbecker felixfbecker merged commit 8a87797 into sourcegraph:master Feb 16, 2018
@daviwil daviwil deleted the typescript-2.7.2 branch February 16, 2018 22:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@felixfbecker felixfbecker felixfbecker approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@daviwil @felixfbecker